Skip to content

bevy_reflect: Use active_types instead of active_fields where appropriate #19922

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jul 14, 2025
Merged

bevy_reflect: Use active_types instead of active_fields where appropriate #19922

merged 2 commits into from
Jul 14, 2025
+13 −19

Conversation

nnethercote
Copy link
Contributor

Objective

Follow on fixes for #19876, using active_types instead of active_fields where appropriate.

Helps a tiny bit with #19873.

Solution

  • Describe the solution used to achieve the objective above.

Testing

I checked the output of cargo expand and ran cargo run -p ci.

@nnethercote nnethercote changed the title Index set Use active_types instead of active_fields where appropriate Jul 2, 2025
@nnethercote
Copy link
Contributor Author

Best reviewed one commit at a time. It looks like this repo might squash-and-rebase to land PRs? I often do multi-commit PRs so I'll give it a try here, see how it pans out.

@tychedelia tychedelia added C-Performance A change motivated by improving speed, memory usage or compile times A-Reflection Runtime information about types S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 2, 2025
@alice-i-cecile
Copy link
Member

Yep, we use squash-on-merge, so the multi-commit approach is really helpful here.

@alice-i-cecile alice-i-cecile requested a review from MrGVSV July 2, 2025 14:52
@nnethercote
Copy link
Contributor Author

Yep, we use squash-on-merge, so the multi-commit approach is really helpful here.

It's a shame that squashing loses the separation in the history. It also decreases the value of bisection, if bugs are introduced.

@nnethercote nnethercote changed the title Use active_types instead of active_fields where appropriate bevy_reflect: Use active_types instead of active_fields where appropriate Jul 2, 2025
@alice-i-cecile
Copy link
Member

Indeed :) However, it is much easier on novice contributors, and it allows us to ensure that every commit in the history compiles via CI checks, allowing for easier bisection and examination of history.

@nnethercote
Copy link
Contributor Author

every commit in the history compiles via CI checks

Oh, interesting. I think I still prefer having the smaller chunks in the history, but reasonably people could disagree and Bevy ain't my project :)

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 13, 2025
@alice-i-cecile
Copy link
Member

@nnethercote can you resolve merge conflicts please?

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jul 13, 2025
@nnethercote
Change WhereClauseOptions::active_fields to active_types
ed1a5ab 
This could have been part of bevyengine#19876, which deduplicated the fields. It's
also simpler and more efficient to keep the (short-lived) active types
as an `IndexSet<Type>`, and avoid converting them to a `Vec<Type>` and
then a `Box<[Type]>`.
@nnethercote
Use active_types in ReflectEnum::get_type_registration.
169307e 
Instead of `active_fields`. This makes it match `ReflectStruct`, and
avoids redundant calls within the generated `register_type_dependencies`
when the same type is used in multiple enum variant fields.
@nnethercote
Copy link
Contributor Author

I rebased. Should be good to go.

@kristoff3r kristoff3r added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 14, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 14, 2025
@alice-i-cecile alice-i-cecile added the C-Code-Quality A section of code that is hard to understand or change label Jul 14, 2025
Merged via the queue into bevyengine:main with commit c994bdf Jul 14, 2025
36 checks passed
@nnethercote nnethercote deleted the IndexSet branch July 14, 2025 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kristoff3r kristoff3r kristoff3r approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@MrGVSV MrGVSV Awaiting requested review from MrGVSV

Assignees
No one assigned
Labels
A-Reflection Runtime information about types C-Code-Quality A section of code that is hard to understand or change C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nnethercote @alice-i-cecile @kristoff3r @tychedelia